Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support multiple LDAP hosts #2776

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Support multiple LDAP hosts #2776

wants to merge 1 commit into from
+110 −18

Conversation

onetwopunch
Copy link

Overview

Often there is a need, as there is in my company, to iterate through multiple LDAP hosts in the event that one has failed. DNS is one possible solution, but there are cases where that may not be possible, for example with Kerberos.

What this PR does / why we need it

This change allows users to specify a comma-separated list of LDAP hosts to iterate through in case one is unreachable. It will function the same as always with one host, but will iterate through if multiple are given and the prior hosts fail.

Special notes for your reviewer

Happy to add tests if necessary, but for such a simple change, I'm not sure it is.

Does this PR introduce a user-facing change?

LDAP connector now supports multiple hosts in a comma-separated list. No changes required.

@onetwopunch onetwopunch force-pushed the feat/multiple-ldap-hosts branch from 3b3730d to d271fb7 Compare January 6, 2023 18:22
@onetwopunch
Support multiple LDAP hosts
3562de8 
Often there is a need to iterate through multiple LDAP hosts in the
event that one has failed. DNS is one possible solution, but there
are cases where that may not be desired. This change allows users to
specify a comma-separated list of LDAP hosts to iterate through in case
one is unreachable.

Signed-off-by: Ryan Canty <[email protected]>
@onetwopunch onetwopunch force-pushed the feat/multiple-ldap-hosts branch from d271fb7 to 3562de8 Compare January 6, 2023 18:24
@onetwopunch
Copy link
Author

@ericchiang I see you worked on the LDAP bits pretty heavily. Any thoughts on this?

@ericchiang
Copy link
Contributor

I haven't worked on dex in years. You'll have to find an actual maintainer to review :(

@onetwopunch
Copy link
Author

Thanks for letting me know and good luck on your current endeavors 😄

@sagikazarmark or @nabokihms any thoughts on this PR?

@dlipovetsky dlipovetsky mentioned this pull request Jan 15, 2023
Open
@dlipovetsky
Copy link
Contributor

dlipovetsky commented Jan 17, 2023 •
edited
Loading

(I'm not a dex maintainer, so please consider this my humble, unsolicited opinion)

The change to the configuration schema is backward compatible. On the one hand, it may be confusing that the Host field supports multiple hosts, from which the first is chosen when the connector is initialized, but, on the other hand, the change is concise, and the common use case (one host) works as it did before.

The change to connector initialization, however, is not backward compatible. Before this change, the connector made no calls to the LDAP host during initialization. With this change, it does, and at least one LDAP host must respond for initialization to succeed. Update: I misread the code. The initialization is backward compatible. Please see comments below.

@onetwopunch
Copy link
Author

@dlipovetsky thanks for the feedback!

The change to connector initialization, however, is not backward compatible

This actually isn't quite . While the connection is checked at startup time, if all of them error out, the last host in the list will be returned regardless of if the connection succeeded, I did this specifically for backwards compatibility.

However, thanks for commenting on this as I realize now that I'm only calling this connection once during startup and not during invocation, which kind of defeats the goal haha.

Not that any maintainers seem interested in this change anyway after 2 weeks of silence. I'll probably just end up forking dex for this and other changes that no one seems interested in addressing 🤷 But thanks again for the feedback!

@dlipovetsky
Copy link
Contributor

While the connection is checked at startup time, if all of them error out, the last host in the list will be returned regardless of if the connection succeeded, I did this specifically for backwards compatibility.

My mistake! 🤦 I see now that you're breaking out of the loop as soon as you get a successful connection.

@dlipovetsky
Copy link
Contributor

However, thanks for commenting on this as I realize now that I'm only calling this connection once during startup and not during invocation, which kind of defeats the goal haha.

A silver lining! 😉

Could you accomplish your goal in

dex/connector/ldap/ldap.go

Lines 310 to 348 in 70e6cc2

// do initializes a connection to the LDAP directory and passes it to the
// provided function. It then performs appropriate teardown or reuse before
// returning.
func (c *ldapConnector) do(_ context.Context, f func(c *ldap.Conn) error) error {
// TODO(ericchiang): support context here
var (
conn *ldap.Conn
err error
)
switch {
case c.InsecureNoSSL:
conn, err = ldap.Dial("tcp", c.Host)
case c.StartTLS:
conn, err = ldap.Dial("tcp", c.Host)
if err != nil {
return fmt.Errorf("failed to connect: %v", err)
}
if err := conn.StartTLS(c.tlsConfig); err != nil {
return fmt.Errorf("start TLS failed: %v", err)
}
default:
conn, err = ldap.DialTLS("tcp", c.Host, c.tlsConfig)
}
if err != nil {
return fmt.Errorf("failed to connect: %v", err)
}
defer conn.Close()
// If bindDN and bindPW are empty this will default to an anonymous bind.
if c.BindDN == "" && c.BindPW == "" {
if err := conn.UnauthenticatedBind(""); err != nil {
return fmt.Errorf("ldap: initial anonymous bind failed: %v", err)
}
} else if err := conn.Bind(c.BindDN, c.BindPW); err != nil {
return fmt.Errorf("ldap: initial bind for user %q failed: %v", c.BindDN, err)
}
return f(conn)
}
?

@onetwopunch
Copy link
Author

Yes I believe I can...I actually started there, but then realized I was making some breaking changes so I switched it but totally missed that that would only do the check at startup 😂 Thanks be to Code Review

@nabokihms nabokihms added the release-note/enhancement Release note: Enhancements label Jan 18, 2023
@nabokihms nabokihms self-requested a review January 18, 2023 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nabokihms nabokihms Awaiting requested review from nabokihms

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
release-note/enhancement Release note: Enhancements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@onetwopunch @ericchiang @dlipovetsky @nabokihms